security(reports): authorize the apps a report targets, on every path that uses them by ar2rsawseen · Pull Request #7864 · Countly/countly-server

ar2rsawseen · 2026-07-29T22:15:31Z

What changes

/i/reports/create and /i/reports/update authorized the target apps only when the effective report_type was "core". A non-core report_type names the plugin that owns the report, and those reports were inserted and updated without any authorization of the object they point at.

The authorization path for this already existed but was unreachable. The dashboards plugin implements /report/authorize, which checks view access to the dashboard a report renders, while the reports-side caller that invoked it (validateNonCoreUser) had been commented out.

This restores that caller and gates both create and update on its result.

Update authorizes the merged stored-plus-payload report, not the payload alone, so a partial update cannot leave an unauthorized target in place and repointing an existing report at a different target is checked too.
A copy is passed, so the authorized flag — which is only how the dispatch returns its result — never reaches the written document.
Fails closed, matching the original intent of the commented-out code: a report type whose plugin does not answer /report/authorize is not authorized. Only "dashboards" implements it today, here and in countly-platform, so a plugin adding a new report type needs to implement the event for it.

Also drops validateCoreUser, which was commented out and superseded by the inline per-app check that uses the reports feature permission rather than plain app membership.

Tests

Real end-to-end coverage through the HTTP endpoints with a real non-admin member, so the whole path runs rather than the branch in isolation:

a member with reports rights but no view access to a private dashboard is refused when scheduling a report against it
the same member succeeds for a dashboard they own, which proves the authorize dispatch resolves rather than the request merely failing
the authorized flag does not appear on the stored report
a core report for an app the member has rights on still succeeds, so the restructuring did not change the core path

Not executed locally, since the harness needs COUNTLY_TEST_API_KEY_ADMIN and COUNTLY_TEST_APP_ID. CI runs them in test-api-plugins.

Propagation

Second commit: the paths that were still owner-only

recordUpdateOrDeleteQuery adds only user, so beyond create and update, four paths were scoped by ownership alone. Owning a report is not the same as still being allowed to read the apps it covers. A member who scheduled a report while they held an app, then lost that access while keeping reports rights elsewhere, could:

re-enable it through /i/reports/status and let the schedule resume,
mail themselves that app's figures on demand through /i/reports/send,
read them straight out of the response through /i/reports/preview and /i/reports/pdf, which render the report and hand it back. This is the most direct of the four: no email involved, the figures come back in the HTTP response.

Nothing re-checked anything at send time either, so a report enabled before access was revoked kept mailing indefinitely with no further action by anybody. The scheduled job calls reports.getReport directly rather than sendReport, so both now consult a shared reports.ownerMayStillSend; guarding only sendReport would have missed the scheduled path entirely, which is the one that matters most.

Deliberately unchanged, with the reasoning recorded in the code:

Disabling stays allowed. It only reduces what the report does, and refusing would leave someone unable to stop mail they no longer want.
Delete stays owner-only. It cannot reach another app's data, and the owner needs a way to clean up.
last_sent is not stamped when a report is skipped, so it resumes on its own if access is restored.

Reports with no apps pass, because those are non-core reports whose target is authorized by their own plugin through /report/authorize rather than per app.

A report whose owner no longer exists also stops being sent. A deleted account holds access to nothing, so the same rule applies to it; the getReport global-admin fallback stays where it is, because preview and pdf rely on it and both are authorized against the member making the request, but it no longer decides whether scheduled mail keeps going out. This matches alerts, which already stopped in that case.

Also drops a stray console.log of the status payload.

Worth a look separately

That global-admin fallback for an unresolvable owner quietly raises the access a report renders with. /i/users/delete removes a departing member's reports, so it should only affect orphaned rows, but I left it alone here rather than change send behaviour inside a security fix.

Tests

Real end-to-end coverage through the HTTP endpoints with a real non-admin member, so the whole path runs rather than the branch in isolation:

a member with reports rights but no view access to a private dashboard is refused when scheduling a report against it
the same member succeeds for a dashboard they own, which proves the authorize dispatch resolves rather than the request merely failing
the authorized flag does not appear on the stored report
a core report for an app the member has rights on still succeeds, so the restructuring did not change the core path

The second commit adds a suite that revokes a member's access to one app mid-test, then covers both directions. The exploit: enable, send and preview are each refused, and the stored report is verified to still be disabled afterwards. The happy paths, which have to keep working: the member can still enable, disable, send and preview reports on the app they do hold, can still switch off and delete the revoked-app report, and a global admin is unaffected.

Not executed locally, since the harness needs COUNTLY_TEST_API_KEY_ADMIN and COUNTLY_TEST_APP_ID. CI runs them in test-api-plugins.

Propagation

Same change is going to release.24.05 and countly-platform main; both carry the identical commented-out caller.

🤖 Generated with Claude Code

/i/reports/create and /i/reports/update authorized the target apps only when the effective report_type was "core". A non-core report_type names the plugin that owns the report, and those reports were inserted and updated without any authorization of the object they point at. The authorization path for this already existed but was unreachable. The dashboards plugin implements /report/authorize, which checks view access to the dashboard a report renders, while the reports-side caller that invoked it (validateNonCoreUser) had been commented out. Restores that caller and gates both create and update on its result. Update authorizes the merged stored-plus-payload report rather than the payload alone, so a partial update cannot leave an unauthorized target in place, and repointing an existing report at a different target is checked too. A copy is passed so the authorize flag, which is only how the dispatch returns its result, never reaches the written document. Fails closed, matching the original intent of the commented-out code: a report type whose plugin does not answer /report/authorize is not authorized. Only "dashboards" implements it today, in this repo and in countly-platform, so a plugin adding a new report type needs to implement the event for it. Also drops validateCoreUser, which was commented out and superseded by the inline per-app check that uses the reports feature permission rather than plain app membership. Co-Authored-By: Claude <noreply@anthropic.com>

Real end-to-end coverage through the HTTP endpoints with a real non-admin member, so the whole path is exercised rather than the branch in isolation: validateCreate, the report_type branch, the /report/authorize dispatch into the dashboards plugin, and the insert. - a member with reports rights but no view access to a private dashboard is refused when scheduling a report against it - the same member succeeds for a dashboard they own, which proves the authorize dispatch resolves rather than the request simply failing - the authorize flag does not appear on the stored report - a core report for an app the member has rights on still succeeds, so the restructuring did not change the core path Not executed locally: the harness needs COUNTLY_TEST_API_KEY_ADMIN and COUNTLY_TEST_APP_ID. CI runs them in test-api-plugins. Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Dashboard permissions are deliberately separate from app permissions: a dashboard can be shared with a member who has no access to the apps its widgets reference, and they are meant to be able to view it and schedule a report for it. The suite proved a member is refused a dashboard they cannot view, and allowed their own, but nothing covered the case in between, which is the one a tightening change could plausibly break. This uses the same admin-owned dashboard the member was already refused, changing only the share, so the test isolates the share as the deciding factor rather than app rights. Also generalised the cleanup to delete a list of dashboards rather than two hard-coded ids. Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Found while auditing for the same class as the alerts fix. The apps list was authorized only when report_type was "core", which left a two step route onto any app: create a report with report_type "dashboards" and apps naming the victim app. The apps check never ran, and the non-core check added earlier authorizes the dashboard target rather than the apps list, so pointing it at your own dashboard passed. update it with report_type "core" and no apps. effectiveType became "core", but the check only looked at the submitted apps, which was absent, so the stored list survived unchecked and the report started using it. Then read it through preview or pdf, or wait for the scheduled mail. Precondition was reports create and update on any single app, with no prior access to the victim app at all. apps is now authorized whenever a list is present, whatever the report type, and on update the list that is authorized is the one the report will actually have: the submitted list when one is sent, the stored list otherwise. Safe for legitimate non-core reports because the dashboards drawer hides the app picker, so those carry an empty apps, and an empty list stays permitted. The shared check also keeps the legacy allowance the surrounding code already makes, so members with no permission object are still authorized through user_of. Tests walk both halves of the chain: create with an unauthorized apps list is refused, and converting a stored report to core is refused when its stored apps name an app the member cannot read. Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

ar2rsawseen · 2026-07-31T12:31:40Z

Scope widened: a second gap in the same handler

An audit for the class behind #7872 turned up a chain in this handler that the original fix here did not close, so it is fixed in this PR rather than a separate one.

The apps list was authorized only when report_type was "core", which left a two step route onto any app:

Create with report_type: "dashboards" and apps: ["<victim>"]. The apps check never ran. The non-core check this PR originally added authorizes the dashboard target, not the apps list, so pointing it at your own dashboard passed.
Update with {_id, report_type: "core"} and no apps. effectiveType became "core", but the check only looked at the submitted apps, which was absent, so the stored list survived unchecked and the report began using it.

Then read it through /i/reports/preview or /i/reports/pdf, or wait for the scheduled mail.

Precondition was reports create and update on any single app, with no prior access to the victim app. I verified both halves against this branch before fixing, so it was live even with the original change.

The fix

apps is authorized whenever a list is present, whatever the report type, and on update the list authorized is the one the report will actually end up with: the submitted list when one is sent, the stored list otherwise.

Why this does not break dashboards reports

The dashboards drawer sets showApps = false and hides the app picker, so a legitimate non-core report carries apps: []. An empty or absent list stays permitted, which is also what keeps report_type: "dashboards" reports for shared dashboards working, including the case where the member has no access to the apps whose widgets the dashboard contains.

The shared helper keeps the legacy allowance the surrounding code already makes, so members with no permission object remain authorized through user_of.

Tests

Two cases walking each half of the chain: create with an unauthorized apps list is refused, and converting a stored report to core is refused when its stored apps name an app the member cannot read. A second app the member has no rights on is created as a fixture and cleaned up.

…nd rendering it The create and update paths now authorize the apps a report will have, but four other paths were scoped by owner alone (recordUpdateOrDeleteQuery adds only `user`). Owning a report is not the same as still being allowed to read the apps it covers, so a member who scheduled a report while they held an app, then lost that access while keeping reports rights elsewhere, could: - re-enable it through /i/reports/status and let the schedule resume, - mail themselves that app's figures on demand through /i/reports/send, - read them straight out of the response through /i/reports/preview and /i/reports/pdf, which render the report and hand it back. Nothing re-checked anything at send time either, so a report enabled before access was revoked kept mailing indefinitely with no further action by anybody. - status authorizes the stored apps before enabling. Disabling stays allowed: it only reduces what the report does, and refusing would leave someone unable to stop mail they no longer want. - send, preview and pdf authorize the stored apps before rendering. - the scheduled job and sendReport both consult reports.ownerMayStillSend, which judges the owner's current access. They reach the renderer by different routes and would otherwise disagree. last_sent is not stamped when a report is skipped, so it resumes by itself if access is restored. - delete stays owner-only on purpose: it cannot reach another app's data, and the owner needs a way to clean up. Two deliberate fail-open choices, to avoid breaking working installations: a report whose owner no longer resolves is logged and still sent, since getReport already falls back to a global admin for it and changing that would stop reports that have worked for years; and reports with no apps pass, because those are non-core reports whose target is authorized by their own plugin through /report/authorize rather than per app. Also drops a stray console.log of the status payload. Tests cover both directions: enable, send and preview are refused for the revoked app and the report is verified to still be disabled, while the member can still enable, disable, send and preview on the app they hold, can still switch off and delete the revoked-app report, and a global admin is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (5)

plugins/reports/api/api.js:655

The status update loop starts iterating user-controlled statusList IDs without guarding for inherited keys or invalid ObjectIDs. Since recordUpdateOrDeleteQuery calls common.db.ObjectID(recordID) directly, a crafted args can throw and potentially crash the request handler. Iterate over Object.keys() and skip invalid IDs safely.

                var bulk = common.db.collection("reports").initializeUnorderedBulkOp();
                for (const id in statusList) {

plugins/reports/tests.js:588

The cleanup in this suite triggers victim app deletion but does not wait for it before proceeding to delete dashboards/user and calling done(). That can leave test artifacts behind (or make later tests flaky if they assume a clean state). Chain the app deletion into the cleanup flow so done() runs after it completes.

                function cleanupRest() {
                    var dashboards = [adminDashboardId, memberDashboardId, sharedDashIdForCleanup].filter(Boolean);
                    if (VICTIM_APP_ID) {
                        request.get('/i/apps/delete?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify({app_id: VICTIM_APP_ID}))).end(function() {});
                    }

plugins/reports/api/reports.js:186

In sendLoadedReport, the error path assumes getReport always supplies an ob object. But getReport sometimes calls back with only an error (no second argument), so ob.message can throw and crash the send path. Guard against ob being undefined when forwarding the error to the callback.

            else if (callback) {
                callback(err2, ob.message);
            }

plugins/reports/api/api.js:474

In the /i/reports/send endpoint, the error log uses err (from the earlier findOne callback) instead of the actual send error err2, so logs will be misleading and make incident triage harder.

This issue also appears on line 654 of the same file.

                            log.d("Error occurred while sending out report.", err);

plugins/reports/tests.js:505

This test relies on an admin /i/reports/update call to plant an unauthorized apps list, but it doesn't assert that the admin update succeeded. If that call fails (e.g. endpoint returns non-200), the rest of the test can become a false positive/negative. Add an expect(200) and fail the test if the setup update errors.

This issue also appears on line 584 of the same file.

                        request.get('/i/reports/update?api_key=' + API_KEY_ADMIN + '&app_id=' + APP_ID
                            + '&args=' + encodeURIComponent(JSON.stringify({_id: created._id, apps: [VICTIM_APP_ID]})))
                            .end(function() {
                                request.get('/i/reports/update?api_key=' + memberApiKey + '&app_id=' + APP_ID
                                    + '&args=' + encodeURIComponent(JSON.stringify({_id: created._id, report_type: "core"})))

…nd rendering it (24.05) Ports the second half of #7864 to this branch. The create and update paths here already authorize the apps a report will have, but the other paths were scoped by owner alone (recordUpdateOrDeleteQuery adds only `user`). Owning a report is not the same as still being allowed to read the apps it covers, so a member who scheduled a report while they held an app, then lost that access while keeping reports rights elsewhere, could: - re-enable it through /i/reports/status and let the schedule resume, - mail themselves that app's figures on demand through /i/reports/send, - read them straight out of the response through /i/reports/preview. Nothing re-checked anything at send time either, so a report enabled before access was revoked kept mailing indefinitely. - status authorizes the stored apps before enabling. Disabling stays allowed: it only reduces what the report does, and refusing would leave someone unable to stop mail they no longer want. - send and preview authorize the stored apps before rendering. - the scheduled job and sendReport both consult reports.ownerMayStillSend, which judges the owner's current access. The job reaches the renderer directly rather than through sendReport, so guarding only sendReport would miss it. last_sent is not stamped when a report is skipped, so it resumes if access is restored. - delete stays owner-only on purpose: it cannot reach another app's data, and the owner needs a way to clean up. The allow-list the create and update paths built inline is extracted to a shared appsArePermitted, rather than repeated three more times. Master carries the same helper, so the two branches now express this the same way. This branch has no /i/reports/pdf endpoint, so that guard from master does not apply here. Two deliberate fail-open choices, to avoid breaking working installations: a report whose owner no longer resolves is logged and still sent, since getReport already falls back to a global admin for it; and reports with no apps pass, because those are non-core reports authorized through /report/authorize instead. Also drops a stray console.log of the status payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Resolves the CHANGELOG conflict by keeping both entries, and adds the missing entry for the second commit's endpoint and send-time authorization. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

plugins/reports/api/reports.js:164

reports.sendReport returns callback(null, null) when ownerMayStillSend is false. Callers (e.g. /i/reports/send) treat a falsy error as success and will respond "Success" even though the report was intentionally not sent, which makes the API outcome inaccurate.

            reports.ownerMayStillSend(db, report, function(maySend) {
                if (!maySend) {
                    return callback(null, null);
                }
                reports.sendLoadedReport(db, report, callback);

plugins/reports/api/api.js:640

In the status path, converting enablingIds to ObjectID inside the try means a single invalid report id in the request turns into a 500 "Failed to change report status". Since the ids come directly from user input, this should fail with a 400 (bad request) and a clear message, while keeping 500 reserved for actual DB errors.

                        toEnable = await common.db.collection("reports").find({
                            _id: {
                                $in: enablingIds.map(function(id) {
                                    return common.db.ObjectID(id);
                                })

A deleted account holds access to nothing, so the rule that stops a report once its owner loses access to the apps it covers applies to a deleted owner too. This branch previously logged a warning and sent anyway, deferring to the global-admin fallback in getReport, which left the one case where nobody at all is authorized as the case that still delivered. That fallback stays where it is, because preview and pdf rely on it and both are authorized against the member making the request. It just no longer decides whether scheduled mail keeps going out. Brings reports in line with alerts, which already stops when the owner cannot be resolved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

plugins/reports/api/api.js:609

The status handler reads const statusList = params.qstring.args; and later iterates for (const id in statusList). If args is missing, this will throw (for-in over undefined) and can fail the request unexpectedly. Defaulting to an empty object prevents runtime errors on malformed requests.

                var params = paramsInstance;
                const statusList = params.qstring.args;

plugins/reports/api/reports.js:186

sendLoadedReport assumes getReport always supplies an ob object. But getReport can call callback(err) with no second argument (e.g. when async.map returns an error), so ob.message here can throw and mask the real error.

            else if (callback) {
                callback(err2, ob.message);
            }

plugins/reports/api/reports.js:162

When ownerMayStillSend returns false, sendReport calls callback(null, null). Callers (e.g. /i/reports/send) will treat this as success even though no email was sent, which makes the endpoint report "Success" on a no-op. Returning a non-null error (or a dedicated status) would avoid false-positive success responses and make skips observable.

            reports.ownerMayStillSend(db, report, function(maySend) {
                if (!maySend) {
                    return callback(null, null);
                }

ar2rsawseen and others added 2 commits July 30, 2026 00:59

Copilot AI review requested due to automatic review settings July 29, 2026 22:15

Copilot AI reviewed Jul 29, 2026

ar2rsawseen mentioned this pull request Jul 29, 2026

security(reports): authorize the apps a report targets, on every path that uses them (24.05) #7865

Open

Copilot AI review requested due to automatic review settings July 30, 2026 14:57

Copilot AI reviewed Jul 30, 2026

Copilot AI review requested due to automatic review settings July 31, 2026 12:31

Copilot AI reviewed Jul 31, 2026

Copilot AI review requested due to automatic review settings August 1, 2026 15:21

Copilot started reviewing on behalf of ar2rsawseen August 1, 2026 15:22 View session

ar2rsawseen changed the title ~~fix(reports): authorize non-core report targets on create and update~~ security(reports): authorize the apps a report targets, on every path that uses them Aug 1, 2026

Copilot AI reviewed Aug 1, 2026

View reviewed changes

Merge branch 'master' into security/reports-noncore-authorization

e4517fd

Resolves the CHANGELOG conflict by keeping both entries, and adds the missing entry for the second commit's endpoint and send-time authorization. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings August 3, 2026 13:43

Copilot started reviewing on behalf of ar2rsawseen August 3, 2026 13:44 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 3, 2026 17:28

Copilot started reviewing on behalf of ar2rsawseen August 3, 2026 17:28 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Navigation Menu

Uh oh!

security(reports): authorize the apps a report targets, on every path that uses them - #7864

security(reports): authorize the apps a report targets, on every path that uses them#7864
ar2rsawseen wants to merge 7 commits into
masterfrom
security/reports-noncore-authorization

ar2rsawseen commented Jul 29, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

ar2rsawseen commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ar2rsawseen commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes

Tests

Propagation

Second commit: the paths that were still owner-only

Worth a look separately

Tests

Propagation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

ar2rsawseen commented Jul 31, 2026

Scope widened: a second gap in the same handler

The fix

Why this does not break dashboards reports

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ar2rsawseen commented Jul 29, 2026 •

edited

Loading